Skip to content

[Form] Add model_type option to MoneyType #19359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

mdoutreluingne
Copy link
Contributor

Fixes #19353

@mdoutreluingne
Copy link
Contributor Author

mdoutreluingne commented Dec 30, 2023

It seems that the names of the options are arranged in alphabetical order, but I'm not sure, can you confirm this?

@OskarStark
Copy link
Contributor

can you confirm this?

Yes

@OskarStark OskarStark changed the title [Form] add "model_type" option to MoneyType [Form] Add model_type option to MoneyType Dec 30, 2023
@mdoutreluingne
Copy link
Contributor Author

Ok, I'll do that in another PR targeting branch 5.4

.. caution::

When the value is set to ``integer``, the ``divisor`` option must not be set to ``1``,
to avoid losing decimals during the casting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason looks confusing to me. How is it possible to lose decimals when the divisor is 1?

Looking at the implementation, it seems more related to the fact that the integer value doesn't have any effect when the divisor is 1.

Copy link
Contributor Author

@mdoutreluingne mdoutreluingne Dec 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misspoke, it is in fact linked to the integer with a divisor is 1.

I find that the caution message is no longer useful. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdoutreluingne I still don't understand this caution. But it's not because of your PR. I've checked the code and I don't understand it either. I also read this discussion and I still don't understand it: symfony/symfony#51884 (comment)

How would you explain this in a way that anybody can understand it? Thanks.

Copy link
Contributor Author

@mdoutreluingne mdoutreluingne Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javiereguiluz

From what I've understood from looking at the implementation, an exception is thrown if the model_type is integer and the divisor is 1, because according to this condition the casting is only done if the divisor is not equal to 1, otherwise it only returns the value entered.

Me too, I don't understand why an exception is thrown if the divisor equals 1. Casting can be done perfectly well if the divisor is equal to 1.

I've added this caution message to warn the reader that if he changes the model_type for integer, he must also change the divisor (because the default divisor value is 1), otherwise an exception will be thrown.

But maybe I've added a message that is ultimately of no interest to the reader and the message of the exception is enough.

What do you think?

Copy link
Member

@javiereguiluz javiereguiluz Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's better to not add this to the docs. Developers will get a explict exception message if they do "wrong".

But, maybe we should revisit this or refactor the code to make it easier to understand. It seems that nobody truly understands this code 😐

Copy link
Contributor Author

@mdoutreluingne mdoutreluingne Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing caution message done.

I agree, I think it should be refactor the code to make it easier to understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened symfony/symfony#53488 to see if we can finally understand the reason of this code. Thanks.

@javiereguiluz javiereguiluz merged commit 7d60d37 into symfony:7.1 Jan 15, 2024
@javiereguiluz
Copy link
Member

Thanks Maxime! I've merged this because your contribution is complete and we don't need to wait to fix symfony/symfony#53488. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Form] add "model_type" option to MoneyType
5 participants